feat: Add snap_startTrace and snap_endTrace support to snaps-jest#3547
feat: Add snap_startTrace and snap_endTrace support to snaps-jest#3547
snap_startTrace and snap_endTrace support to snaps-jest#3547Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3547 +/- ##
=======================================
Coverage 98.27% 98.28%
=======================================
Files 415 417 +2
Lines 11717 11765 +48
Branches 1821 1827 +6
=======================================
+ Hits 11515 11563 +48
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }); | ||
| } | ||
|
|
||
| case 'trace': { |
There was a problem hiding this comment.
I had to add this because we clear the trackables state for every request. We could consider not clearing performance traces between requests as well.
There was a problem hiding this comment.
Would it be a lot of work to keep pendingTraces between runs?
There was a problem hiding this comment.
Unsure whether most usage would be during a single request tbh 🤔
There was a problem hiding this comment.
We explicitly clear it, so it would be easy to keep it. Just unsure about the expectations.
There was a problem hiding this comment.
Do we have no existing state between requests?
There was a problem hiding this comment.
Right now all the notifications and trackables are cleared:
snaps/packages/snaps-simulation/src/request.ts
Lines 104 to 105 in f08611a
But Snap state isn't.
There was a problem hiding this comment.
Hmm. Up to you whether you think it makes sense to persist it between runs, we can always easily change the behavior.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }, | ||
| endTrace: (state, action: PayloadAction<EndTraceParams>) => { | ||
| const endTrace = castDraft(action.payload); | ||
| const index = state.pendingTraces.findIndex( |
There was a problem hiding this comment.
Do we need logic to handle multiple pending traces with the same name/id?
There was a problem hiding this comment.
Other than changing it to LIFO like Cursor suggested, I'm not sure what we should change. Unless you think new traces should override existing ones if the name/ID is the same? I have no idea how Sentry handles it, but generally I would expect that Snaps don't run multiple traces at the same time with the same name.
There was a problem hiding this comment.
Do we need to match on both name and ID? Are we expecting that Snaps will pass both (our example does not)?
There was a problem hiding this comment.
The name is required, but ID is optional. For matching, just passing a name in both cases will work (since undefined === undefined), but if the ID in the end request is different it will fail.
There was a problem hiding this comment.
Perhaps fair enough to expect the same parameters on both sides
There was a problem hiding this comment.
But I think there is a chance we have to adjust this based on usage
| clearTrackables: (state) => { | ||
| state.events = []; | ||
| state.errors = []; | ||
| state.traces = []; |
There was a problem hiding this comment.
| state.traces = []; | |
| state.traces = []; | |
| // We are not resetting `pendingTraces` to let them persist between requests. |
There was a problem hiding this comment.
Not sure if this is necessary since it's documented in the test as well
2 participants
This adds support for
snap_startTraceandsnap_endTracetosnaps-jest, and atoTracematcher to go with it.